-
Notifications
You must be signed in to change notification settings - Fork 25.6k
[ML] Refactoring inference API throttled logging #124923
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ML] Refactoring inference API throttled logging #124923
Conversation
|
|
||
| import org.apache.logging.log4j.LogManager; | ||
| import org.apache.logging.log4j.Logger; | ||
| import org.elasticsearch.common.Strings; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would ignore the previous version of this class and review this like it's a new file. It has changed pretty significantly.
| } | ||
|
|
||
| this.cancellableTask.set(startResetTask(threadPool)); | ||
| public void init() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The init() class are to avoid a very unlikely this leak if the object hadn't finished it construction yet.
…ve-throttled-logging
…ve-throttled-logging
…ve-throttled-logging
| final ReentrantReadWriteLock.WriteLock writeLock = lock.writeLock(); | ||
|
|
||
| return threadPool.scheduleWithFixedDelay(logExecutors::clear, resetInterval, threadPool.executor(UTILITY_THREAD_POOL_NAME)); | ||
| writeLock.lock(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Locks so that we don't get inconsistencies while the logging the repeated messages. Otherwise the counts could be incorrect.
| final String stringToAppend = messageToAppend; | ||
| return new LogExecutor(this.clock, 0, (message) -> executor.accept(message.concat(stringToAppend))); | ||
| private void log(String enrichedMessage) { | ||
| LogBuilder builder = throttledLogger.atLevel(level); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I originally tried constructing the LogBuilder in the manager class but it will fail to log because the LogBuilder log() method must be called on the constructing thread.
|
Pinging @elastic/ml-core (Team:ML) |
💚 Backport successful
|
* Refactoring throttled logging * Renaming setting * Resetting log counter when attempt to log for repeated messages * Clean up * Refactoring locking * Comments * Working tests * Addressing logbuilder issue * Fixing tests
…5338) * [ML] Refactoring inference API throttled logging (#124923) * Refactoring throttled logging * Renaming setting * Resetting log counter when attempt to log for repeated messages * Clean up * Refactoring locking * Comments * Working tests * Addressing logbuilder issue * Fixing tests * Removing version annotations
* Refactoring throttled logging * Renaming setting * Resetting log counter when attempt to log for repeated messages * Clean up * Refactoring locking * Comments * Working tests * Addressing logbuilder issue * Fixing tests
* Refactoring throttled logging * Renaming setting * Resetting log counter when attempt to log for repeated messages * Clean up * Refactoring locking * Comments * Working tests * Addressing logbuilder issue * Fixing tests
* Refactoring throttled logging * Renaming setting * Resetting log counter when attempt to log for repeated messages * Clean up * Refactoring locking * Comments * Working tests * Addressing logbuilder issue * Fixing tests
This PR refactors the throttled logging logic in a few ways. The new functionality is the following:
Throttlerclass is initialized which runs on an interval (default is 1 hour) and will emit any messages that had been repeatedTesting
To test the logic, attempt to create an invalid openai inference endpoint
This will cause a message to be emitted. If you send the same PUT request it will not be emitted. This is because the message matched so the counter will be incremented.
To force the message to be emitted we can update the logging interval to a shorter time like 5 seconds
After 5 seconds you should see the message.
Examples: